[Core] Fix #27497: Improve CommandIndex case-insensitive lookup performance#32418
Closed
DanielMicrosoft wants to merge 2 commits intoAzure:devfrom
Closed
[Core] Fix #27497: Improve CommandIndex case-insensitive lookup performance#32418DanielMicrosoft wants to merge 2 commits intoAzure:devfrom
DanielMicrosoft wants to merge 2 commits intoAzure:devfrom
Conversation
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
Collaborator
|
Thank you for your contribution! We will review the pull request and get back to you soon. |
|
The git hooks are available for azure-cli and azure-cli-extensions repos. They could help you run required checks before creating the PR. Please sync the latest code with latest dev branch (for azure-cli) or main branch (for azure-cli-extensions). pip install azdev --upgrade
azdev setup -c <your azure-cli repo path> -r <your azure-cli-extensions repo path>
|
DanielMicrosoft
changed the title
DanielMicrosoft
requested review from
bebound,
calvinhzy,
evelyn-ys,
jiasli,
jsntcy,
kairu-ms,
necusjz and
zhoxing-ms
as code owners
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR improves the performance of command index lookups by ensuring case-insensitive command matching. The fix prevents unnecessary command index rebuilds when users enter commands with mixed casing (e.g., az Version vs az version).
Key Changes
- Added
.lower()normalization to the top-level command during command index lookup - Introduced comprehensive test coverage for case-insensitive command lookups
- Updated VS Code debug configurations to use the newer
debugpytype
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| src/azure-cli-core/azure/cli/core/init.py | Normalizes command lookup to lowercase to prevent cache misses due to casing differences |
| src/azure-cli-core/azure/cli/core/tests/test_command_registration.py | Adds test coverage for case-insensitive command index lookups with lowercase, uppercase, and mixed case scenarios |
| .vscode/launch.json | Updates debug configurations to use debugpy type and removes deprecated debugOptions, but introduces invalid JSON syntax with trailing commas |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
The reason why #27545 is not merge is that we believe users shouldn't use uppercase in commands. |
Contributor
Author
hmm ok, thanks for context. Although it doesn't make much sense, as CLI itself will allow any casing when it tries to execute the command, it just means if someone uses any casing other than all lower case, they will get an arbitrary performance hit as we constantly try to re-build the commandIndex? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related command
Description
ADO Task: https://msazure.visualstudio.com/One/_workitems/edit/35842223
Fixes #27497
Closes #27545
This PR implements the same fix proposed in #27545, which has been inactive for over two years. Reopening a fresh PR was simpler and ensures the changes align with the current codebase.
Testing Guide
See ADO task for testing results.
You can see the issue by testing commands with mixed casing (ie
az Version) vs (az version) and observing that command index will be re-built in all scenarios when using mixed casing (and obviously a time penalty).There are comments from @jiasli in Hang's original PR questioning the correct place to normalize arg's, and the
load_command_tablemethod takesargsand appliesroughly_parse_command, there's an argument to be had about applying that earlier (perhaps as soon as args are received in main.py), however that would increase the blast radius of the changes.This is a targeted approach designed to only target commandIndex lookups to prevent cache misses based on casing.
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.